Skip to content

fix(#4060): add bounded jittered backoff to StartableToolSet retry path - #4062

Open
aheritier wants to merge 1 commit into
mainfrom
fix/startable-toolset-backoff
Open

fix(#4060): add bounded jittered backoff to StartableToolSet retry path#4062
aheritier wants to merge 1 commit into
mainfrom
fix/startable-toolset-backoff

Conversation

@aheritier

@aheritier aheritier commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

🤖 Automated implementer agentthis comment was posted by the implementer bot from Docker Agentic Platform, not by a human developer

Refs #4060 (partial — MCP/LSP startup errors and RAG 5xx/408 pacing deferred to follow-up)

What

Adds bounded, cancellable, jittered exponential backoff to StartableToolSet's non-blocking start path. A rate-limited embedding-provider 429 that previously triggered a full concurrent re-index on every agent turn is now paced to at most one retry per 15 s – 5 min window.

Design

Gate location — tryStartLocked (TryStart/TryStartWithTimeout only). Blocking Start() bypasses the gate so mcpcatalog enable and skill sub-session startup always get a fresh attempt.

Classifier — startBackoffRetryable. Requires a *modelerrors.StatusError in the error chain via errors.As. Port numbers and chunk-progress counters in plain error strings cannot arm the gate.

Bounds — base 15 s, cap 5 min, additive jitter [d, 1.2d]. The additive floor guarantees the full nominal wait; jitter de-synchronises concurrent toolset sources.

Retry-After. When the 429 response carries a Retry-After header, that hint overrides the computed delay (capped at 5 min, with the same additive jitter applied to avoid re-synchronisation).

Embedding-provider gap fixed. openai/client.go and dmr/embed.go now call oaistream.WrapOpenAIError on their embedding errors. WrapOpenAIError wraps *openaisdk.Error in *modelerrors.StatusError carrying the HTTP status code, so a 429 from the embedding provider reaches the gate as *StatusError and arms it.

Known limitation. Code-mode composites (codemode.Wrap) return PartialStartError on a partial failure; the partial-start branch resets the gate and leaves the failed-subset retry unpaced. Tracked in #4067.

Changed files

File What changed
pkg/modelerrors/modelerrors.go New exported RetryableHTTPStatus(err) classifier
pkg/modelerrors/modelerrors_test.go Tests for the classifier (incl. regex false-positive cases)
pkg/tools/startable_backoff.go New: constants, computeStartBackoff, startBackoffRetryable, additiveJitter, retryAfterHint
pkg/tools/startable_backoff_test.go Unit tests: gate fires on 429/408/5xx, not on plain text; Retry-After honoured/ignored/capped; jitter spread
pkg/tools/startable_backoff_regression_test.go Consumer-shaped regression suite (RAG/MCP/LSP shapes, no-leak, latch, blocking Start ungated)
pkg/tools/startable.go tryStartLocked, StartableOption, WithStartRetryJitter, WithStartRetryClock, variadic NewStartable
pkg/tools/export_test.go Exported helpers for deterministic test control
pkg/tools/builtin/rag/rag_backoff_test.go Real-toolset integration test via rag.New + fake clock
pkg/model/provider/openai/client.go Wrap batch-embedding error in oaistream.WrapOpenAIError
pkg/model/provider/openai/embed_test.go httptest 429 → proves *StatusError chain; 5xx equivalent
pkg/model/provider/dmr/embed.go Same — wrap DMR embedding error in oaistream.WrapOpenAIError
pkg/model/provider/dmr/embed_test.go httptest 429 → proves *StatusError chain
pkg/agent/agent.go Update stale log messages
pkg/tools/mcp/mcp.go Update stale log message
docs/tools/rag/index.md New section: trigger table (429-only), retry schedule, Retry-After note, troubleshooting
docs/tools/mcp/index.md Lifecycle note: local startup failures are not paced by the gate
docs/tools/lsp/index.md Same note for LSP
docs/community/troubleshooting/index.md Cross-reference to RAG backoff section

@aheritier
aheritier marked this pull request as ready for review August 27, 2026 09:30
@aheritier
aheritier requested a review from a team as a code owner August 27, 2026 09:30
@aheritier aheritier added area/tools For features/issues/fixes related to the usage of built-in and MCP tools kind/fix PR fixes a bug (maps to fix:). Use on PRs only. labels Aug 27, 2026
aheritier

This comment was marked as resolved.

@aheritier
aheritier marked this pull request as draft August 27, 2026 12:58
aheritier added a commit that referenced this pull request Aug 27, 2026
…rt, align bounds

Addresses all blocking and should-fix findings from the aheritier review
on PR #4062:

[blocking #1 + #2] Generic classifier with HTTP-status precedence:
- Add modelerrors.RetryableHTTPStatus(err) — catches any error carrying a
  retryable HTTP status (429/408/5xx) via *StatusError or message regex,
  without string-pattern heuristics ('connection refused' stays non-retryable).
- startBackoffRetryable becomes: return err != nil && RetryableHTTPStatus(err).
  A StatusError{429} coexisting with context.DeadlineExceeded now arms the
  gate (HTTP wins), fixing the deadline-masks-rate-limit race.

[blocking #3] Bounds aligned with remediation plan:
- base = 15s, cap = 5min (was 1s/30s).
- Additive jitter [d, 1.2d] (was equal jitter [d/2, d]), guaranteeing the
  full nominal wait is always respected.

[blocking #4] Gate enforced only in the TryStart path:
- Move gate check from startLocked into new tryStartLocked (called by
  TryStart/TryStartWithTimeout only).
- Start() calls startLocked directly — mcpcatalog enable and skill
  sub-session startup are never delayed.

[should-fix #5] External recovery via StartReporter:
- tryStartLocked checks reporter.IsStarted() when started==false; a live
  reporter (e.g. after /toolset-restart) clears the gate and latches the
  wrapper without calling the underlying Start.
- New test: TestStartableToolSet_ExternalRecoveryClearsBackoffGate.

[should-fix #6] Exported constructor options for cross-package tests:
- NewStartable(ts, opts...) with StartableOption, WithStartRetryJitter,
  WithStartRetryClock.
- nowFn() clock seam; zero-value StartableToolSet still usable.

[should-fix #7] Concurrent and at-boundary tests:
- TestStartableToolSet_BackoffNoDoubleStartWithinWindow: 20 goroutines
  calling TryStart, assert underlying Start invoked exactly once.
- TestStartableToolSet_BackoffAtBoundary: fake clock, gate open at expiry.

[optional] Stale comment name in BackoffDormantForPlainErrors fixed.

Also:
- RetryableHTTPStatus test cases include plain-text regex fallback.
- ExportedSetClock removed (unused; WithStartRetryClock preferred).
- All gating tests converted from s.Start() to s.TryStart().
- Jitter-bounds assertions updated to [nominal, 1.2×nominal].

PR2 (#4065) will need rebasing and test updates after this lands.
@aheritier
aheritier marked this pull request as ready for review August 27, 2026 13:46
aheritier

This comment was marked as resolved.

aheritier added a commit that referenced this pull request Aug 27, 2026
[blocking] partial-start comment corrected; known limitation for code-mode
composites documented with reference to follow-up issue #4067

[SF1] startBackoffRetryable requires *StatusError — regex fallback excluded
to prevent false positives on port numbers and chunk counters; RetryableHTTPStatus
doc corrected to describe the actual regex-fallback behaviour

[SF2] TestStartableToolSet_BlockingStartSkipsGate: arm gate, assert blocking
Start() invokes underlying (not gated), assert TryStart() is gated

[SF3] tryStartLocked: reporter-adoption guarded by !startBackoffUntil.IsZero()
— fix is now scoped to gated state; non-gated TryStart semantics unchanged

[SF4] stale 'next turn' log messages updated in agent.go and mcp.go

[optional] ExportedSetJitter removed (dead code; WithStartRetryJitter preferred)
aheritier added a commit that referenced this pull request Aug 27, 2026
…rt, align bounds

Addresses all blocking and should-fix findings from the aheritier review
on PR #4062:

[blocking #1 + #2] Generic classifier with HTTP-status precedence:
- Add modelerrors.RetryableHTTPStatus(err) — catches any error carrying a
  retryable HTTP status (429/408/5xx) via *StatusError or message regex,
  without string-pattern heuristics ('connection refused' stays non-retryable).
- startBackoffRetryable becomes: return err != nil && RetryableHTTPStatus(err).
  A StatusError{429} coexisting with context.DeadlineExceeded now arms the
  gate (HTTP wins), fixing the deadline-masks-rate-limit race.

[blocking #3] Bounds aligned with remediation plan:
- base = 15s, cap = 5min (was 1s/30s).
- Additive jitter [d, 1.2d] (was equal jitter [d/2, d]), guaranteeing the
  full nominal wait is always respected.

[blocking #4] Gate enforced only in the TryStart path:
- Move gate check from startLocked into new tryStartLocked (called by
  TryStart/TryStartWithTimeout only).
- Start() calls startLocked directly — mcpcatalog enable and skill
  sub-session startup are never delayed.

[should-fix #5] External recovery via StartReporter:
- tryStartLocked checks reporter.IsStarted() when started==false; a live
  reporter (e.g. after /toolset-restart) clears the gate and latches the
  wrapper without calling the underlying Start.
- New test: TestStartableToolSet_ExternalRecoveryClearsBackoffGate.

[should-fix #6] Exported constructor options for cross-package tests:
- NewStartable(ts, opts...) with StartableOption, WithStartRetryJitter,
  WithStartRetryClock.
- nowFn() clock seam; zero-value StartableToolSet still usable.

[should-fix #7] Concurrent and at-boundary tests:
- TestStartableToolSet_BackoffNoDoubleStartWithinWindow: 20 goroutines
  calling TryStart, assert underlying Start invoked exactly once.
- TestStartableToolSet_BackoffAtBoundary: fake clock, gate open at expiry.

[optional] Stale comment name in BackoffDormantForPlainErrors fixed.

Also:
- RetryableHTTPStatus test cases include plain-text regex fallback.
- ExportedSetClock removed (unused; WithStartRetryClock preferred).
- All gating tests converted from s.Start() to s.TryStart().
- Jitter-bounds assertions updated to [nominal, 1.2×nominal].

PR2 (#4065) will need rebasing and test updates after this lands.
aheritier added a commit that referenced this pull request Aug 27, 2026
Regression suite for the backoff gate introduced in PR1 (#4062).

Test files:
- pkg/tools/startable_backoff_regression_test.go: 9 consumer-shaped
  regression tests using fakes modelled on real RAG/MCP/LSP error shapes —
  RAG-shaped failure+recovery, MCP/LSP compatibility (fail-fast, no backoff),
  HTTP 408 gate, concurrent starts no-multiplication, no timer/goroutine leak,
  cancellation no-window, jitter de-synchronization, already-started no-restart.
- pkg/tools/builtin/rag/rag_backoff_test.go: 2 real-toolset RAG tests using
  rag.New + countingStatusErrStrategy; proves the real toolset's StatusError
  wrapping chain is traversable by errors.As and that plain errors fail fast.

Docs:
- docs/tools/rag/index.md: authoritative 'Indexing failures, retries and
  backoff' section — retry policy (1s base, 30s cap, equal jitter), what
  triggers backoff (429, 408, 5xx) vs fail-fast (other 4xx, cancellation),
  operational impact and troubleshooting guidance.
- docs/tools/mcp/index.md: short note under Lifecycle clarifying MCP local
  startup failures fail fast; links to RAG page for full policy.
- docs/tools/lsp/index.md: matching note under Auto-Restart and Lifecycle.

No production code changes.
@aheritier
aheritier force-pushed the fix/startable-toolset-backoff branch from ce13fa7 to ab320cf Compare August 27, 2026 15:48
aheritier added a commit that referenced this pull request Aug 27, 2026
[blocking] partial-start comment corrected; known limitation for code-mode
composites documented with reference to follow-up issue #4067

[SF1] startBackoffRetryable requires *StatusError — regex fallback excluded
to prevent false positives on port numbers and chunk counters; RetryableHTTPStatus
doc corrected to describe the actual regex-fallback behaviour

[SF2] TestStartableToolSet_BlockingStartSkipsGate: arm gate, assert blocking
Start() invokes underlying (not gated), assert TryStart() is gated

[SF3] tryStartLocked: reporter-adoption guarded by !startBackoffUntil.IsZero()
— fix is now scoped to gated state; non-gated TryStart semantics unchanged

[SF4] stale 'next turn' log messages updated in agent.go and mcp.go

[optional] ExportedSetJitter removed (dead code; WithStartRetryJitter preferred)
@aheritier
aheritier requested a review from docker-agent August 27, 2026 15:52
@aheritier
aheritier force-pushed the fix/startable-toolset-backoff branch from ab320cf to 6310940 Compare August 27, 2026 19:37
aheritier

This comment was marked as resolved.

@aheritier
aheritier force-pushed the fix/startable-toolset-backoff branch 2 times, most recently from 501cab1 to 7d9852d Compare August 28, 2026 10:43
aheritier

This comment was marked as resolved.

@aheritier
aheritier force-pushed the fix/startable-toolset-backoff branch from 7d9852d to b318089 Compare August 28, 2026 12:06
@aheritier

This comment was marked as resolved.

@aheritier
aheritier force-pushed the fix/startable-toolset-backoff branch from b318089 to 1991438 Compare August 28, 2026 13:41
@aheritier

This comment was marked as resolved.

@aheritier
aheritier force-pushed the fix/startable-toolset-backoff branch from 1991438 to fe986e7 Compare August 28, 2026 14:46
@aheritier aheritier changed the title fix: add bounded jittered backoff to StartableToolSet retry path fix(#4060): add bounded jittered backoff to StartableToolSet retry path Aug 28, 2026
@aheritier

This comment was marked as outdated.

@aheritier
aheritier force-pushed the fix/startable-toolset-backoff branch from fe986e7 to e0b4aab Compare August 28, 2026 15:56
aheritier

This comment was marked as resolved.

Refs #4060 (partial): RAG semantic-embeddings indexing triggered a
rate-limit retry storm — repeated toolset-start attempts had no pacing
after a 429 from the embedding provider (the 15s/5min gate is the partial
fix; DefaultStartTimeout and chunk-level checkpointing are deferred).

Implementation:
- modelerrors.RetryableHTTPStatus(err): HTTP-status classifier that
  recognises 429, 408, and 5xx via *StatusError first, then falls back
  to statusCodeRegex. The toolset gate pre-filters to *StatusError via
  errors.As before calling it, so port numbers and chunk counts in
  plain error strings cannot arm the gate.
- pkg/tools/startable_backoff.go: bounded exponential backoff with
  additive 0-20% jitter (base=15s, cap=5min, delay∈[d,1.2d]).
- Gate in tryStartLocked (TryStart/TryStartWithTimeout only): blocking
  Start() bypasses it so mcpcatalog enable and skill startup are
  immediate.
- Gate adopts a live StartReporter after /toolset-restart without
  waiting for the window to expire.
- Wrap embedding errors via oaistream.WrapOpenAIError at openai/client.go
  and dmr/embed.go so a 429 from the embedding provider surfaces as
  *StatusError and correctly arms the gate.
- WithStartRetryJitter / WithStartRetryClock options via variadic
  NewStartable for deterministic test control.
- Stale 'retry on next turn' log messages updated in agent.go/mcp.go.
- Partial-start exemption documented (code-mode composites remain
  unpaced; follow-up at issue #4067).

Tests (same commit, covering the above):
- startable_backoff_test.go: unit tests for the gate (gate fires on
  429/408/5xx StatusError, not on plain text / context errors,
  blocking Start() ungated, concurrency, jitter bounds)
- startable_backoff_regression_test.go: consumer-shaped regression
  suite (RAG/MCP/LSP error shapes, no-goroutine/timer leak, latch)
- rag_backoff_test.go: real-toolset integration test via rag.New +
  fake clock

Docs:
- docs/tools/rag/index.md: 'Indexing failures, retries and backoff'
  section with trigger table, parameters, and troubleshooting.
- docs/tools/mcp/index.md, docs/tools/lsp/index.md: lifecycle notes
  confirming local startup failures fail fast.

Scope: DefaultStartTimeout (30s) unchanged — deferred.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/tools For features/issues/fixes related to the usage of built-in and MCP tools kind/fix PR fixes a bug (maps to fix:). Use on PRs only.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant